-
-
Notifications
You must be signed in to change notification settings - Fork 64
Allow disabling trailers #809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| val fs2GrpcDisableTrailers = | ||
| settingKey[Boolean]("Disable generation of the trailers") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, we can extend CodeGeneratorOption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would that look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add a new option:
object CodeGenerationOptions {
...
case object Fs2GrpcDisableTrailers extends CodeGeneratorOption {
override def toString: String = "fs2_grpc:disable_trailers"
}
}Here is a complete example: iRevive#1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Not sure if that is preferable. Then we have two ways of customizing fs2grpc things, one for the serviceSuffix and one for fs2_grpc:disable_trailers, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If we want to follow the new code-generation option path, we should deprecate fs2GrpcServiceSuffix and replace it with CodeGeneratorOption.Fs2GrpcServiceSuffix(suffix: String).
But I'm not sure it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If CodeGeneratorOption is the recommendation then I think it is worth it. Now is the time to do breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, let's move to CodeGeneratorOption then.
I will move fs2GrpcServiceSuffix to the CodeGeneratorOption in the follow-up PR. Let's deprecate the fs2GrpcServiceSuffix plugin key and keep it around for a while so that users can migrate.
Motivation
In some cases, trailer classes are not needed and only add compilation overhead, especially with many services. They remain enabled by default, but an option is now available to disable them.